Skip to content

[VPEX][6/8] Add local-env uv backend and hidden CLI command#5832

Open
rugpanov wants to merge 7 commits into
mainfrom
dbconnect/06-command
Open

[VPEX][6/8] Add local-env uv backend and hidden CLI command#5832
rugpanov wants to merge 7 commits into
mainfrom
dbconnect/06-command

Conversation

@rugpanov

@rugpanov rugpanov commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Why

  • The engine needs a real PackageManager implementation (uv) and a CLI entry point so a user can actually run the feature.
  • Wiring it in while keeping it Hidden lets the command be dogfooded and exercised by acceptance tests without becoming user-visible until the stack is complete.
  • This is the first layer reachable from main, so it also makes the whole libs/localenv package live for the deadcode checker.

What

  • libs/localenv/uv.go — the uv implementation of PackageManager: discover/install uv, install the Python minor, uv sync, seed pip into the venv, validate; plus the pip.confUV_INDEX_URL bridge for Databricks-managed machines.
  • cmd/localenv/ — the command tree matching local-env python sync: a local-env group (Hidden: true), a python subgroup, and the sync verb. Parent nodes use root.ReportUnknownSubcommand; sync uses cobra.NoArgs, resolves flags/bundle target, builds the Pipeline with the uv manager, and renders text or --json. All Cobra Use values + the --json command field come from the libs/localenv constants.
  • cmd/cmd.go — registers the group.

Testing strategy

  • Unit tests for uv helper logic (discovery, pip.conf index-url, arg builders, stderr surfacing) (uv_test.go).
  • Runtime smoke test of the hidden 3-level command: absent from top-level help; help works at each level; unknown subcommand exits non-zero; bare group shows help; flags + mutual-exclusion + NoArgs behave.
  • Gates: go build ./..., go test, golangci-lint, deadcode (whole tree, no pragmas), gofmt — all green.
  • cmd/localenv/ unit tests are intentionally deferred to the acceptance PR ([VPEX][7/8] Add local-env acceptance tests #5833), per the repo convention that user-visible CLI output is covered by acceptance tests.

About this stack

This is one of a series of small, stacked PRs that together add the databricks local-env python sync command — it provisions a local Python environment (Python version, databricks-connect pin, and dependency constraints) matched to a selected Databricks compute target. The work was split from one large branch into single-concern layers so each is independently reviewable; the command is kept hidden until the final PR so nothing is user-visible mid-stack.

Review bottom-up. Each PR targets the previous one as its base branch (this PR targets main), so its diff shows only that layer. Layers 1–5 have merged; the original layer-5 PR (#5828) was split into 5a/5b/5c during review.

# PR What Status
1 #5823 foundation: result types + env-key mapping merged
2 #5824 compute-target resolution merged
3 #5826 constraint fetch + offline cache merged
4 #5827 formatting-preserving pyproject.toml merge merged
5a #5850 package-manager interface + detection merged
5b #5851 six-phase pipeline orchestrator merged
5c #5854 --check cache purity, greenfield name, dbc insertion merged
6 #5832 ← you are here uv backend + CLI command (registered hidden)
7 #5833 acceptance tests
8 #5835 unveil (unhide + help + changelog)

This pull request and its description were written by Isaac.

@eng-dev-ecosystem-bot

eng-dev-ecosystem-bot commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Integration test report

Commit: 11a1285

Run: 29345764311

Env 🔄​flaky 💚​RECOVERED 🙈​SKIP ✅​pass 🙈​skip Time
💚​ aws linux 4 4 230 1087 5:24
💚​ aws windows 4 4 232 1085 8:06
💚​ aws-ucws linux 4 4 316 1004 5:34
💚​ aws-ucws windows 4 4 318 1002 7:34
💚​ azure linux 4 4 230 1086 5:19
🔄​ azure windows 2 2 4 232 1084 9:07
💚​ azure-ucws linux 4 4 318 1001 5:54
💚​ azure-ucws windows 4 4 320 999 7:32
💚​ gcp linux 4 4 229 1088 5:06
💚​ gcp windows 4 4 231 1086 7:17
8 interesting tests: 4 SKIP, 2 RECOVERED, 2 flaky
Test Name aws linux aws windows aws-ucws linux aws-ucws windows azure linux azure windows azure-ucws linux azure-ucws windows gcp linux gcp windows
💚​ TestAccept 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
🙈​ TestAccept/bundle/invariant/no_drift 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_endpoints/drift/recreated_same_name 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_indexes/recreate/embedding_dimension 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/ssh/connection 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
💚​ TestFetchRepositoryInfoAPI_FromRepo 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
🔄​ TestFetchRepositoryInfoAPI_FromRepo/root 💚​R 💚​R 💚​R 💚​R 💚​R 🔄​f 💚​R 💚​R 💚​R 💚​R
🔄​ TestFetchRepositoryInfoAPI_FromRepo/subdir 💚​R 💚​R 💚​R 💚​R 💚​R 🔄​f 💚​R 💚​R 💚​R 💚​R
Top 10 slowest tests (at least 2 minutes):
duration env testname
6:39 aws windows TestAccept
6:29 aws-ucws windows TestAccept
6:24 azure-ucws windows TestAccept
6:22 gcp windows TestAccept
6:16 azure windows TestAccept
2:58 gcp linux TestAccept
2:57 azure linux TestAccept
2:50 aws linux TestAccept
2:48 aws-ucws linux TestAccept
2:48 azure-ucws linux TestAccept

@rugpanov rugpanov force-pushed the dbconnect/05-pipeline branch from d8f0de9 to de8581b Compare July 6, 2026 11:47
@rugpanov rugpanov force-pushed the dbconnect/06-command branch from 2fd09fe to ae5563c Compare July 6, 2026 11:47
@rugpanov rugpanov temporarily deployed to test-trigger-is July 6, 2026 11:48 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 6, 2026 11:48 — with GitHub Actions Inactive
@rugpanov rugpanov force-pushed the dbconnect/05-pipeline branch from de8581b to be1b6b3 Compare July 7, 2026 08:35
@rugpanov rugpanov force-pushed the dbconnect/06-command branch from ae5563c to a7fa50c Compare July 7, 2026 08:35
@rugpanov rugpanov temporarily deployed to test-trigger-is July 7, 2026 08:35 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 7, 2026 08:35 — with GitHub Actions Inactive
ZaSkittles pushed a commit to ZaSkittles/cli that referenced this pull request Jul 7, 2026
databricks#5823)

## Why

- The `local-env` feature needs a shared vocabulary before any behavior
can be built: the result shape, the error taxonomy, and how a compute
target maps to an environment key.
- Landing these contract types first lets every later layer (resolve /
fetch / merge / pipeline / command) depend on stable, reviewed
definitions.
- Kept deliberately minimal and dependency-free so it reviews on its own
and stays `unused`/`deadcode`-clean with no consumers yet.

## What

- **`result.go`** — the `--json` / `E_*` output contract: `Result`,
`PipelineError`, `ErrorCode`, `PhaseName`, `PhaseStatus`, `Mode`,
`TargetInfo`, `ResolvedInfo`, `Plan`, `Warning`; plus the command-path
constants (`local-env` / `python` / `sync`) defined in one place.
- **`envkey.go`** — `EnvKeyForServerless` / `EnvKeyForSparkVersion` /
`NormalizeServerless`, and `PythonMinorFromRequires` (clause-aware:
returns the effective highest lower bound of a `requires-python`).
- No wiring into `cmd/`, so the CLI is unchanged. Filesystem/artifact
constants and the phase-order slice deliberately live with their
consumer (PR 5).

## Testing strategy

- Unit tests for the error/type contract (`result_test.go`) and env-key
mapping incl. multi-clause / strict-`>` / no-floor `requires-python`
cases (`envkey_test.go`).
- Gates: `go build`, `go test`, `golangci-lint`, `deadcode`, `gofmt` —
all green.
- Reviewed with codex across several rounds to convergence (all findings
fixed or explicitly rejected as speculative).

---

## About this stack

This is one of a series of small, stacked PRs that together add the
`databricks local-env python sync` command — it provisions a local
Python environment (Python version, `databricks-connect` pin, and
dependency constraints) matched to a selected Databricks compute target.
The work was split from one large branch into single-concern layers so
each is independently reviewable; the command is kept hidden until the
final PR so nothing is user-visible mid-stack.

**Review bottom-up.** Each PR targets the previous one as its base
branch, so its diff shows only that layer.

| # | PR | What |
|---|----|------|
| 1 | **databricks#5823 ← you are here** | foundation: result types + env-key
mapping |
| 2 | databricks#5824 | compute-target resolution |
| 3 | databricks#5826 | constraint fetch + offline cache |
| 4 | databricks#5827 | formatting-preserving pyproject.toml merge |
| 5 | databricks#5828 | six-phase pipeline + detection + package-manager interface
|
| 6 | databricks#5832 | uv backend + CLI command (registered hidden) |
| 7 | databricks#5833 | acceptance tests |
| 8 | databricks#5835 | unveil (unhide + help + changelog) |

This pull request and its description were written by Isaac.
ZaSkittles pushed a commit to ZaSkittles/cli that referenced this pull request Jul 7, 2026
## Why

- `local-env` must turn the user's compute selection into a single
environment key before it can fetch anything, and the selection can come
from several places with a defined precedence.
- Isolating resolution behind a narrow seam keeps it testable without a
live workspace and keeps SDK details out of the engine.

## What

- **`target.go`** — `ResolveTarget` with ordered precedence `--cluster`
→ `--serverless` → `--job` → bundle target, producing a `TargetInfo` +
env key.
- Compute lookups go through the narrow `ComputeClient` interface
(stubbable in tests).
- `ValidateTargetFlags` rejects more than one target flag;
`ResolveTarget` runs it up front so a non-Cobra caller can't silently
resolve the wrong target.
- Classic-compute jobs read the Spark version from the documented first
return of `GetJobSparkVersion` (not the recorded-version third return).

## Testing strategy

- Unit tests against a stub `ComputeClient` covering each precedence
branch, the mutually-exclusive-flags error, and the job classic-compute
contract (`target_test.go`).
- Gates: `go build`, `go test`, `golangci-lint`, `deadcode`, `gofmt` —
all green.
- Reviewed with codex to a clean pass.

---

## About this stack

This is one of a series of small, stacked PRs that together add the
`databricks local-env python sync` command — it provisions a local
Python environment (Python version, `databricks-connect` pin, and
dependency constraints) matched to a selected Databricks compute target.
The work was split from one large branch into single-concern layers so
each is independently reviewable; the command is kept hidden until the
final PR so nothing is user-visible mid-stack.

**Review bottom-up.** Each PR targets the previous one as its base
branch, so its diff shows only that layer.

| # | PR | What |
|---|----|------|
| 1 | databricks#5823 | foundation: result types + env-key mapping |
| 2 | **databricks#5824 ← you are here** | compute-target resolution |
| 3 | databricks#5826 | constraint fetch + offline cache |
| 4 | databricks#5827 | formatting-preserving pyproject.toml merge |
| 5 | databricks#5828 | six-phase pipeline + detection + package-manager interface
|
| 6 | databricks#5832 | uv backend + CLI command (registered hidden) |
| 7 | databricks#5833 | acceptance tests |
| 8 | databricks#5835 | unveil (unhide + help + changelog) |

This pull request and its description were written by Isaac.
ZaSkittles pushed a commit to ZaSkittles/cli that referenced this pull request Jul 7, 2026
…icks#5826)

## Why

- Once a target resolves to an env key, `local-env` needs the pinned
Python version, `databricks-connect` version, and dependency constraints
published for that key.
- The fetch must degrade gracefully offline and distinguish “this
environment isn't published” from “the network is down,” because those
call for different user action.
- The artifact host must be a Databricks-owned, access-controlled
location and must never default to a personal repo — whoever controls
the host controls what the CLI installs.

## What

- **`constraints.go`** — fetches the per-environment `pyproject.toml`,
parses `requires-python`, the `databricks-connect` pin, and `[tool.uv]`
`constraint-dependencies`, and caches it on disk.
- **Host parameterization** — no host is hardcoded:
`RepoConstraintBaseURL` reads the repo (`owner/name`) from the temporary
`DATABRICKS_LOCALENV_CONSTRAINT_REPO` env var and builds a
`raw.githubusercontent.com/<repo>/main` URL; the built-in default is
empty. When unset it returns `""` and `FetchConstraints` reports the
missing source as a fetch-phase `E_FETCH` error (so there is no
untrusted default, and the failure flows through the normal phase/JSON
reporting). Once `databricks/environments` can publish, that becomes the
hardcoded default and the env var is no longer required.
- Failure classification: **404** → `E_ENV_UNSUPPORTED` (no cache
fallback — a distinct non-transient condition); **transport / non-404**
→ `E_FETCH` with fallback to the last-good cached copy.
- Robustness: validate the body (parse + require `requires-python`)
**before** caching so a bad 2xx can't poison the cache; atomic cache
write (mkdir + temp-file + rename); dedicated `http.Client` with a 30s
timeout; body read bounded by `io.LimitReader` at 1 MiB;
`databricks-connect` matched by leading package name under PEP 503
normalization (so `Databricks_Connect` matches, `databricks-connectors`
does not); cache filename = readable slug + sha256 suffix to prevent
collisions.

## Testing strategy

- Unit tests with an `httptest` server: 200-parse, 404 →
`E_ENV_UNSUPPORTED`, transport failure + cache fallback,
missing-`requires-python` rejection, PEP 503 name matching, cache-dir
creation, collision-free filenames, oversized-body rejection
(`constraints_test.go`).
- Host resolution: `TestRepoConstraintBaseURL` (env var → URL, unset →
`""`, whitespace treated as unset) and
`TestFetchConstraintsNoSourceConfigured` (empty host → `E_FETCH` naming
the env var).
- Gates: `go build`, `go test`, `golangci-lint`, `deadcode`, `gofmt` —
all green.
- Reviewed with codex to a clean pass (several fetch/cache edge-case
fixes landed from review).

---

## About this stack

This is one of a series of small, stacked PRs that together add the
`databricks local-env python sync` command — it provisions a local
Python environment (Python version, `databricks-connect` pin, and
dependency constraints) matched to a selected Databricks compute target.
The work was split from one large branch into single-concern layers so
each is independently reviewable; the command is kept hidden until the
final PR so nothing is user-visible mid-stack.

**Review bottom-up.** Each PR targets the previous one as its base
branch, so its diff shows only that layer.

| # | PR | What |
|---|----|------|
| 1 | databricks#5823 | foundation: result types + env-key mapping |
| 2 | databricks#5824 | compute-target resolution |
| 3 | **databricks#5826 ← you are here** | constraint fetch + offline cache |
| 4 | databricks#5827 | formatting-preserving pyproject.toml merge |
| 5 | databricks#5828 | six-phase pipeline + detection + package-manager interface
|
| 6 | databricks#5832 | uv backend + CLI command (registered hidden) |
| 7 | databricks#5833 | acceptance tests |
| 8 | databricks#5835 | unveil (unhide + help + changelog) |

This pull request and its description were written by Isaac.
ZaSkittles pushed a commit to ZaSkittles/cli that referenced this pull request Jul 7, 2026
…atabricks#5827)

## Why

- `local-env` must apply the resolved Python version and constraints to
the user's `pyproject.toml` without disturbing their own content —
comments, ordering, formatting, and unrelated config must survive
untouched.
- Re-running must be safe and idempotent, and a greenfield project needs
a sensible file created from scratch.
- This is the most intricate logic in the feature, so it lands in its
own PR for focused review.

## What

- **`merge.go`** — a formatting-preserving merge that rewrites only the
env-owned regions (`requires-python`, the `databricks-connect` entry in
`[dependency-groups].dev`, and a marker-bracketed managed `[tool.uv]`
block) and preserves every other byte incl. CRLF; idempotent.
`RenderFreshPyproject` builds a complete managed file for a greenfield
project.
- Scoping/robustness: managed `constraint-dependencies` nests
header-less inside an existing user `[tool.uv]` (never a duplicate
header); single- vs multi-line array detection tracks real bracket depth
outside strings/comments; the `databricks-connect` rewrite is confined
to `dev` and leaves trailing comments alone; `requires-python`'s inline
comment is preserved; table-header parsing tolerates inline comments and
recognizes `[[array.of.tables]]`.

## Testing strategy

- Unit tests that parse the merged output as TOML (not just substring
checks), covering idempotency, CRLF preservation, user-key preservation,
the duplicate-`[tool.uv]` case, bracket-in-element arrays,
sibling-group/comment non-clobbering, and `[[tool.uv.index]]` children
(`merge_test.go`).
- Gates: `go build`, `go test`, `golangci-lint`, `deadcode`, `gofmt` —
all green.
- Reviewed with codex to a clean pass (multiple TOML-corruption edge
cases were caught and fixed).

---

## About this stack

This is one of a series of small, stacked PRs that together add the
`databricks local-env python sync` command — it provisions a local
Python environment (Python version, `databricks-connect` pin, and
dependency constraints) matched to a selected Databricks compute target.
The work was split from one large branch into single-concern layers so
each is independently reviewable; the command is kept hidden until the
final PR so nothing is user-visible mid-stack.

**Review bottom-up.** Each PR targets the previous one as its base
branch, so its diff shows only that layer.

| # | PR | What |
|---|----|------|
| 1 | databricks#5823 | foundation: result types + env-key mapping |
| 2 | databricks#5824 | compute-target resolution |
| 3 | databricks#5826 | constraint fetch + offline cache |
| 4 | **databricks#5827 ← you are here** | formatting-preserving pyproject.toml
merge |
| 5 | databricks#5828 | six-phase pipeline + detection + package-manager interface
|
| 6 | databricks#5832 | uv backend + CLI command (registered hidden) |
| 7 | databricks#5833 | acceptance tests |
| 8 | databricks#5835 | unveil (unhide + help + changelog) |

This pull request and its description were written by Isaac.
Sixth in the stacked series. Wires the engine to a runnable CLI command,
registered Hidden so it does not appear in help or completion until the final
unveil PR (it is invocable for dogfooding in the meantime).

- libs/localenv/uv.go: the uv implementation of the PackageManager interface
  (discover/install uv, install Python, uv sync, seed pip, validate the venv),
  plus the pip.conf -> UV_INDEX_URL bridge for Databricks-managed machines.
- cmd/localenv: the command tree matching the target path
  "local-env python sync" — a top group (local-env), a python subgroup, and the
  sync verb. Parent nodes use root.ReportUnknownSubcommand so an unknown
  subcommand errors while a bare group shows help. sync resolves flags/bundle
  target, builds the Pipeline with the uv manager, and renders text or --json.
- cmd/cmd.go: register the group (Hidden:true).

The sync verb rejects stray positional args (cobra.NoArgs) rather than silently
ignoring them, since the target is chosen via flags. Job compute resolution
reads job-level environments/job_clusters; task-level compute is out of scope
and returns an actionable error pointing at --cluster/--serverless.

This is the first layer reachable from main, so it makes the whole
libs/localenv package live for the deadcode checker without any pragmas. Build,
unit tests, lint, and deadcode are clean; the hidden three-level command was
smoke-tested end to end (help at each level, unknown-subcommand errors, flags).

Co-authored-by: Isaac
@rugpanov rugpanov force-pushed the dbconnect/06-command branch from a7fa50c to f1ef8fd Compare July 14, 2026 11:51
@rugpanov rugpanov changed the base branch from dbconnect/05-pipeline to main July 14, 2026 11:52
Fixes from codex + isaac review of the uv backend:
- Windows uv auto-install used the Unix sh/curl installer, which is
  unavailable in a default Windows shell. Use the PowerShell installer
  on Windows.
- discoverUv skipped only the literal "/uv"/"" degenerate paths, so
  with $HOME unset the relative candidates (e.g. "uv") were os.Stat'd
  against the CWD and a stray ./uv could be executed. Skip any
  non-absolute candidate via filepath.IsAbs.
- pipConfIndexURL only read the Linux ~/.config/pip/pip.conf path,
  missing pip's macOS (~/Library/Application Support) and Windows
  (%APPDATA%\pip\pip.ini) config locations, so the corporate index-url
  bridge silently failed on those platforms.

Co-authored-by: Isaac
@rugpanov rugpanov temporarily deployed to test-trigger-is July 14, 2026 12:50 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 14, 2026 12:50 — with GitHub Actions Inactive
Second-pass codex review of the uv backend:
- discoverUv probed only the bare name "uv" in the well-known install
  locations, but the Windows installer writes uv.exe there, so after a
  successful install discovery still returned E_UV_MISSING. Probe the
  OS-appropriate binary name.
- A job that declares both serverless environments and classic job
  clusters was silently classified as serverless (env v4), which can
  provision the wrong local environment. Return an actionable
  ambiguity error instead of guessing.

Co-authored-by: Isaac
@rugpanov rugpanov temporarily deployed to test-trigger-is July 14, 2026 12:56 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 14, 2026 12:56 — with GitHub Actions Inactive
…ster jobs

Third-pass review of the uv backend:
- resolveIndexURL logged the pip.conf index URL verbatim at --debug;
  private PyPI proxies commonly embed credentials in userinfo, so the
  secret leaked into debug logs. Redact userinfo before logging.
- pipConfPaths missed the legacy ~/.pip/pip.conf location pip still
  reads, so managed machines using that layout fell back to pypi.org.
- GetJobSparkVersion used the first job cluster unconditionally; a job
  whose clusters declare differing spark_version is ambiguous, so return
  an actionable error instead of provisioning for the wrong runtime.

Co-authored-by: Isaac
@rugpanov rugpanov temporarily deployed to test-trigger-is July 14, 2026 13:04 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 14, 2026 13:04 — with GitHub Actions Inactive
The task test jobs on PR #5832 failed across Linux, macOS, and Windows:

- TestResolveIndexURLRespectsExistingEnv/returns_pip_conf_url_when_
  UV_INDEX_URL_unset (all platforms): CI's build environment exports
  UV_INDEX_URL to the package mirror, and env.Lookup falls back to
  os.LookupEnv, so resolveIndexURL took the "already set" branch and
  returned "". Setting it to "" in context is not enough (Lookup still
  reports ok=true), so the subtest now removes it from the process
  environment via t.Setenv + os.Unsetenv (t.Setenv restores it on cleanup).

- TestPipConfIndexURL/returns_url_from_pip_conf (Windows): the test wrote
  $HOME/.config/pip/pip.conf, but pipConfPaths probes %APPDATA%\pip\pip.ini
  on Windows, so the file was never read. A new writePipConf helper writes
  to the OS-specific primary path returned by pipConfPaths.

- TestDiscoverUvFindsBinOnPath (Windows): the on-PATH binary was named
  "uv" with no extension, but exec.LookPath only resolves a bare name to a
  file with a PATHEXT extension on Windows. It is now named uv.exe there.

Test-only change; no production code is touched.

Co-authored-by: Isaac
@rugpanov rugpanov temporarily deployed to test-trigger-is July 14, 2026 15:30 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 14, 2026 15:30 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 14, 2026 15:32 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 14, 2026 15:32 — with GitHub Actions Inactive
@rugpanov rugpanov requested a review from anton-107 July 14, 2026 15:51

@anton-107 anton-107 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — this is a clean, well-structured layer: it builds, libs/localenv unit tests pass, the error taxonomy is tidy, and the comments are unusually good at explaining why. Nothing here blocks merge.

One thing I'd genuinely like addressed before this command stops being hidden, plus a few smaller follow-ups (left as inline comments).

⚠️ Please address before unhide: silent curl | sh auto-install of uv

EnsureAvailableinstallUv runs sh -c "curl -LsSf https://astral.sh/uv/install.sh | sh" (and powershell … irm … | iex on Windows) the first time uv isn't found — with no prompt, no opt-in flag, and not even a log.Debugf line before it fires. A user's first local-env python sync silently downloads and executes a remote installer that mutates ~/.local/bin. This is the one I'd push on:

  • At minimum, log the install (and the URL) before running it.
  • Better: prompt when interactive (the repo already has cmdio.IsPromptSupported / the prompt pattern in cmd/labs/project/login.go), and require an explicit opt-in / documented env var in non-interactive contexts. Note libs/versioncheck only ever suggests the equivalent curl string rather than executing it — worth staying consistent with that posture.

Other findings (inline)

  • Bundle is loaded twice, and even when an explicit --cluster/--serverless/--job flag makes the bundle result irrelevant — can also double-print bundle load-time diagnostics.
  • Text-mode failures print the error message twice (phase line + root's Error: line); the JSON path already avoids this via ErrAlreadyPrinted.
  • Serverless --job always resolves to serverless-v4 even though the SDK exposes the environment version on the job.
  • A few cleanups: triplicated index-URL branch, single-caller newUvManager wrapper, positional Validate stdout parsing.

Verified as NON-issues (so nobody re-litigates)

  • JobClusters[0].NewCluster.SparkVersion — no nil panic; SDK JobCluster.NewCluster is a value type.
  • errors.AsType with the stdlib errors import is correct on Go 1.26.
  • resolveIndexURL returning "" when UV_INDEX_URL is already set is correct — process.Background merges env.All(ctx) first, so the existing value still propagates.
  • pipIndexURLRe correctly ignores extra-index-url, comments, and index_url; redactURLCredentials strips userinfo correctly.
  • The PR-description "runtime smoke test" wording isn't misleading given the adjacent line that explicitly defers cmd tests to #5833.

Nice work overall — the merge/backup safety comments in the earlier layers carry through well here.

Comment thread libs/localenv/uv.go
func (m *uvManager) EnsureAvailable(ctx context.Context) (string, error) {
bin, err := discoverUv(ctx)
if err != nil {
if installErr := installUv(ctx); installErr != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Please address before this is unhidden. This silently runs curl -LsSf https://astral.sh/uv/install.sh | sh (and irm … | iex on Windows) the first time uv isn't found — no prompt, no opt-in flag, and not even a log.Debugf before it executes. The user's first local-env python sync downloads and runs a remote installer that mutates ~/.local/bin with no signal.

Suggestions, in order of preference:

  1. Prompt when interactive (cmdio.IsPromptSupported + the prompt pattern in cmd/labs/project/login.go), and require an explicit opt-in / documented env var in non-interactive runs.
  2. At the very least, log.Infof/log.Debugf the install and the URL before running it, so --debug shows what happened.

For comparison, libs/versioncheck only ever surfaces the equivalent curl command as a string to the user rather than executing it — worth matching that posture here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a log.Debugf of the exact installer command + URL before it runs (visible under --debug). The interactive prompt + non-interactive opt-in env var will be addressed in a follow-up, before the command is unhidden.

Comment thread cmd/localenv/sync.go
//
// TODO: extend once bundle config exposes a serverless field at the bundle level.
func bundleTarget(cmd *cobra.Command) libslocalenv.BundleTarget {
b := root.TryConfigureBundle(cmd)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PreRunE = root.MustWorkspaceClient already calls TryConfigureBundle (running DefaultMutators + target selection + building the workspace client). configureBundle never stores the *bundle.Bundle on the context — bundle.Context() has no production callers — so bundle.GetOrNil returns nil here and this second TryConfigureBundle does a full second load.

Two consequences:

  • Because logdiag renders immediately (Collect=false), any bundle load-time warning/recommendation is printed to the user twice.
  • bundleTarget runs unconditionally, but ResolveTarget returns from the --cluster/--serverless/--job branch before ever consulting the bundle — so for the common explicit-flag case the whole second load is wasted work.

Suggest skipping bundleTarget when an explicit target flag is set, and/or reusing the bundle already resolved in PreRunE. (Impact is bounded — bundle load is fast/no-network and the command is hidden — so not a blocker.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — deferring to a follow-up. Will skip bundleTarget when an explicit target flag is set / reuse the bundle from PreRunE.

Comment thread cmd/localenv/output.go Outdated

if pipelineErr != nil {
cmdio.LogString(ctx, "For more detail, re-run with --debug, or --output json to share a structured report.")
return pipelineErr

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Text-mode failures print the message twice. pipeline.fail() sets the failing phase's Detail = pe.Error(), which the phase loop above prints inline (resolve error target resolution failed: <cause>); then returning the raw *PipelineError here means errors.Is(err, ErrAlreadyPrinted) is false in root.Execute (its Unwrap() yields the wrapped cause, not the sentinel), so root prints Error: <same message> again.

The JSON branch above already handles this by returning root.ErrAlreadyPrinted; the text path should do the same after emitting the phase lines + hint, so the failure shows up once.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring to a follow-up: will read Environments[i].Spec.EnvironmentVersion for serverless jobs instead of defaulting to serverless-v4.

Comment thread cmd/localenv/compute.go

// Serverless jobs have Environments populated; classic compute uses JobClusters.
if len(job.Settings.Environments) > 0 {
return "", true, "", nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a serverless job this returns version="", and ResolveTarget treats an empty serverless version as "default to v4" — so every serverless --job resolves to serverless-v4, silently, even for a job pinned to another environment version. Unlike the bundle path (where the version genuinely isn't recorded), the data is present here: job.Settings.Environments[i].Spec.EnvironmentVersion.

I realize the v4 default is a documented stand-in from the original script (spec §4.3) and matches the bundle path, so this may be deliberate for P0 — but since the job path could do better, either read EnvironmentVersion or leave a short TODO noting the job path has more info than the bundle path.

Comment thread libs/localenv/uv.go
if err != nil {
return "", "", uvFailure(ErrValidate, err, "uv run python validation")
}
lines := strings.Split(strings.TrimSpace(out), "\n")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor/robustness: this parse is positional — lines[0] is assumed to be the Python version and the last line the databricks-connect version. CRLF and the dbc-absent case are handled fine, but any stray line uv writes to stdout before the interpreter output would make lines[0] wrong (e.g. "warning: …\n3.12\n17.2.0" → python "warning: …"). uv diagnostics normally go to stderr, so this is unlikely in practice — but a sentinel-delimited print (print("PYVER:" + …) and grep for the prefix) would make it robust rather than order-dependent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, will make the Validate parse sentinel-delimited rather than positional in a follow-up.

Comment thread libs/localenv/uv.go

// newUvManager returns a uvManager whose binary path is resolved lazily via
// EnsureAvailable.
func newUvManager() *uvManager {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: newUvManager is a single-caller wrapper whose body is just return &uvManager{}, and only NewUvManager calls it (tests build &uvManager{} literals directly). Could fold it into NewUvManager and move the lazy-binary-path doc comment there.

Separately (not worth its own thread): EnsurePython/Provision/PostProvision each repeat the identical resolveIndexURL + if indexURL != "" { WithEnv } else {} shape — collapsible into one runUv(ctx, args, dir) helper (WithDir("") and WithEnvs(emptyMap) are both no-ops, so behavior is preserved; the conditional itself must stay so an already-set UV_INDEX_URL isn't clobbered).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fold newUvManager into NewUvManager and collapse the repeated resolveIndexURL branch into a runUv helper in a follow-up.

Two review comments from the PR (both left as non-blocking):

- cmd/localenv/output.go: text-mode failures printed the error twice. The
  phase loop already prints the failing phase's Detail (Pipeline.fail sets
  it to the error text), then returning the raw *PipelineError made root
  print "Error: <same>" again, because PipelineError.Unwrap yields the
  cause rather than the ErrAlreadyPrinted sentinel. Return
  root.ErrAlreadyPrinted like the JSON branch already does, so the failure
  shows once and still exits non-zero.

- libs/localenv/uv.go: installUv runs a remote installer (curl|sh, or
  irm|iex on Windows) that mutates ~/.local/bin. Log the exact command and
  URL before it runs so --debug shows what happened.

Co-authored-by: Isaac
@rugpanov rugpanov temporarily deployed to test-trigger-is July 15, 2026 12:13 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 15, 2026 12:13 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants